Stop write_skippable_frame reading past the end of its input - #144

ext/zstdruby/skippable_frame.c:

size_t dst_size = input_size + ZSTD_SKIPPABLEHEADERSIZE + skip_size;
VALUE output = rb_str_new(input_data, dst_size);

dst_size is the destination size, but input_data holds only input_size bytes — so this copies 8 + skip_size bytes past the end of the first argument's String buffer.

Why removing the copy is safe

The copied bytes never survive. ZSTD_writeSkippableFrame writes the frame at offset 0, and rb_str_resize(output, output_size) then truncates the result to 8 + skip_size. So input_value's contents are discarded either way — only its length participates, via dst_size.

rb_str_new(NULL, dst_size) allocates the same buffer without reading anything, which is exactly what rb_read_skippable_frame does a few lines below.

Reproducing

Public API only, no GC involved — a plain out-of-bounds read:

require "zstd-ruby"
data = "A" * 8                       # tiny input, so the over-read runs off its end
skip = "B" * (1 << 20)
Zstd.write_skippable_frame(data, skip)

Graded by over-read size, 3/3 each, both builds made in the same step that ran the test:

skip_size   over-read      main       with this patch
64          72 B           ok         ok
4096        4104 B         ok         ok
65536       65544 B        ok         ok
1 MiB       1,048,584 B    SEGV       ok
8 MiB       8,388,616 B    SEGV       ok

The threshold is the point at which the read leaves the mapped region — smaller over-reads silently return adjacent heap, which is why this hasn't shown up as a crash before. Ruby 4.0.6, in a container.

Unrelated observation, not changed here

While writing the test I noticed the return value doesn't contain the compressed data, despite the README naming it compressed_data_with_skippable_frame:

c = Zstd.compress("hello world" * 10)   # 27 bytes
out = Zstd.write_skippable_frame(c, "sample data")
out.bytesize        # => 19, i.e. 8 + "sample data".bytesize
out.include?(c)     # => false
Zstd.decompress(out) # => raises

So the frame replaces the input rather than being prepended to it. That may well be intended — I've left it alone, since changing it would be a behaviour change rather than a memory-safety fix. Flagging it in case the README is what's wrong.

Found while sweeping native gems for dangling pointers and out-of-bounds reads.